chore(implementation): use Jetty-12.1 core without servlets #333
Conversation
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided. BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12. Signed-off-by: Lachlan Roberts <[email protected]>
lachlan-roberts
changed the title
lachlan-roberts
changed the title
|
Thanks! Also all references with maven.compiler.source>11</maven.compiler.source> need to change to 17 |
|
Maybe add java21 or 25 |
|
@akerekes please help on google internal kokoro workflows. |
|
Hi @lachlan-roberts , thank you for the PR! I'll work on the Google internal side. One note on the timeline is that Cloud Function deprecates java11 at the end of the month: https://cloud.google.com/functions/docs/runtime-support#java so my preference is to have this PR ready to merge, but wait until the end of the month with the merge, then do it in early November and release a new version of the FF-java library. |
There was a problem hiding this comment.
Thank you @lachlan-roberts for the PR! I did the first round of review.
Please rebase the PR to HEAD one more time, I'm not planning to merge any other PRs until this one is done to avoid further rebasing and merge conflicts.
| private static Event parseLegacyEvent(Request req) throws IOException { | ||
| try (BufferedReader bodyReader = new BufferedReader( | ||
| new InputStreamReader(Content.Source.asInputStream(req), | ||
| Objects.requireNonNullElse(Request.getCharset(req), StandardCharsets.ISO_8859_1)))) { |
There was a problem hiding this comment.
Would UTF-8 charset work here?
| res.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | ||
| logger.log(Level.SEVERE, "Failed to execute " + functionExecutor.functionName(), t); | ||
| res.setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500); | ||
| callback.succeeded(); |
There was a problem hiding this comment.
Should this be callback.failed()?
| import javax.servlet.ServletException; | ||
| import javax.servlet.http.HttpServletRequest; | ||
| import javax.servlet.http.Part; | ||
| import org.eclipse.jetty.http.*; |
There was a problem hiding this comment.
Can you replace this with specific class imports?
| if (inputStream != null) { | ||
| throw new IllegalStateException("getInputStream already called"); | ||
| } |
There was a problem hiding this comment.
Can you move this to the beginning of the method so that the body of getInputStream() and getReader() look consistent?
| @Override | ||
| public InputStream getInputStream() throws IOException { | ||
| return part.getInputStream(); | ||
| // TODO: update with createContentSource when https://github.com/jetty/jetty.project/pull/13610 is released. |
There was a problem hiding this comment.
Looks like this is released now
| public boolean handle(Request request, Response response, Callback callback) { | ||
| try { | ||
| testReference.get().test(new HttpRequestImpl(req)); | ||
| if (!HttpMethod.POST.is(request.getMethod())) { |
There was a problem hiding this comment.
Can you add a comment to mention that this restriction is just the result of the migration from servlets and if a test case required this handler could be extended to accept GET requests.
| @Override | ||
| protected void doPost(HttpServletRequest req, HttpServletResponse resp) { | ||
| public boolean handle(Request request, Response response, Callback callback) { | ||
| if (!HttpMethod.POST.is(request.getMethod())) { |
There was a problem hiding this comment.
Can you add the same comment here re: POST vs GET as above?
| @Override | ||
| protected void doPost(HttpServletRequest req, HttpServletResponse resp) { | ||
| public boolean handle(Request request, Response response, Callback callback) { | ||
| if (!HttpMethod.POST.is(request.getMethod())) { |
There was a problem hiding this comment.
Any reason why this handler handles non-POST requests differently than above?
| callback.succeeded(); | ||
| } catch (Throwable t) { | ||
| exceptionReference.set(t); | ||
| Response.writeError(request, response, callback, t); |
There was a problem hiding this comment.
Could the above handler do the same instead of t.printStacktrace()?
| response -> response.setStatusCode(HttpStatus.IM_A_TEAPOT_418), | ||
| response -> assertThat(response.getStatus()).isEqualTo(HttpStatus.IM_A_TEAPOT_418)), | ||
| responseTest( | ||
| // reason string cannot be set by application |
There was a problem hiding this comment.
IIUC this comment would be better above line 469 explaining why the assertion is against Code.IM_A_TEAPOT.getMessage() and not against Je suis une théière
| <maven.compiler.source>17</maven.compiler.source> | ||
| <maven.compiler.target>17</maven.compiler.target> | ||
| <cloudevents.sdk.version>4.0.1</cloudevents.sdk.version> | ||
| <jetty.version>12.1.1</jetty.version> |
3 participants
This is based on the work from #235.
Port the invoker to upgrade to Eclipse Jetty-12 version 12. Specifically using the new core APIs of Eclipse Jetty-12 that allow the overhead of a Servlet container to be avoided.
BREAKING CHANGE: use Java 17 or above, as required by Eclipse Jetty-12.
Replaces